Skip to content

Miscellaneous YamlChan bug fixes and test coverage - #66

Open
embray wants to merge 15 commits into
Starlink:masterfrom
embray:yamlchan-keymap-proxy
Open

Miscellaneous YamlChan bug fixes and test coverage#66
embray wants to merge 15 commits into
Starlink:masterfrom
embray:yamlchan-keymap-proxy

Conversation

@embray

@embray embray commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

This is built on top of #55 (otherwise ready to merge). I got kind of carried away with several things:

  • Reworks on top of Add astGetKeyMap protected method to Object #55 to use the new Object-attached KeyMap to store additional data about transforms read from ASDF
  • This is used especially for the divide transform to simplify round-tripping it from/to YAML without going through the awkward FindDivide code path.
  • Simplified how the spherical_cartesian transform is handled; my original implementation overcomplicated things and was at odds with out other transforms handled angular unit conversions (which I didn't fully understand at the time I implemented it).
  • Added several more tests for YamlChan. My initial goal was to try to get to 80% coverage, but for now will stop at ~76%. There are a few more cases that could be hit fairly easily but the returns in terms of line count are rapidly diminishing.
  • Fixed a few small bugs caught thanks to the added tests.

@embray
embray force-pushed the yamlchan-keymap-proxy branch from 3c2bdfa to 923db75 Compare July 3, 2026 11:40
embray added 14 commits July 3, 2026 13:41
It wasn't easy to follow what exactly was being tested here, so I split
each logical functionality being tested into separate unit tests, in
preparation to add more.
This is useful for checking coverage of the tests themselves locally
(previously they were excluded from the lcov report).  coverage analysis
on tests is good to make sure the tests are being run as expected, don't
have dead code, etc.

I added some LCOV_EXCL_LINE in testobject.c for the astError calls that
*should* never be reached under normal operating, boosting the overall
coverage to 99%
- astGetKeyMap returns a new reference now via astClone, not a borrowed
  reference, so caller has to astAnnul
- account for this->key_map in GetObjSize and ManageLock
- in the tests, realized I should use astSame to compare object
  identity, rather than direct pointer comparison; while the latter
  works when only testing the internal API that's an implementation
  detail--if we later want to change this to a public API the test will
  still have the correct semantics
This allows testing if an Object already has an associated KeyMap
without implicitly creating one as astGetKeyMap does.
Instead of setting a KeyMap as the transform object's 'proxy' via
astSetProxy.  The goal is to be able to not lose this data even when
cloning the Mapping objects.

The larger goal is to be able to assign the proxy data when reading a
transform in initially -- this way it can already be fixed, and
round-trip losslessly without having to go through the structural
Find<Transform> heuristics.  This commit was originally going to be more
extensive but I ran into problems with the angular unit conversions
which needs to be carefully reconsidered first as a follow-up.
- When I first implemented this I tried to structurally match a SphMap
  flanked by ZoomMaps as an ASDF spherical_cartesian; turns out this is
  the wrong approach and actually overcomplicates matters with the
  angular unit handling.  This ran counter to, e.g., how FindRotate3d
  works, in that it only looks for the core Mapping in radians, and
  then handles the necessary unit flanking on write (with possible
  simplification via SimplifyAsdf).  For SphericalCartesian it's
  sufficient to just write a bare SphMap to ASDF spherical_cartesian.

- Fixed a possible heap overflow bug in SimplifyAsdf I found while
  trying to understand and debug this code.
These are only the versions I found in my test files; probably several
other transforms have had their versions increased.  Will follow-up
later with a more comprehensive review of the latest transform schema
versions.
At read-time (from ASDF) immediately attach PROXY_TYPE and additional
summary to the returned Mapping.  This can be used to round-trip
directly back to an ASDF divide transform without the structural
matching of FindDivide.  The latter is still kept, though it's a pattern
that's unlikely to occur in any Mapping list that wasn't read from ASDF
in the first place.
Adds new tests covering Affine transforms, FK5 frames with time equinox
(covering time parsing), Quantity arrays

TODO: See if anything can be fixed about the roudning on Jyear parsing.
The property has always been called 'intercept', not 'offset'.

Extends the quantity test case to test scalar properties, exercising the
GetQuantity function, and adds several more tests for 1- and 2-D
transforms, which is how the linear1d bug was caught.

Thing I learned: There is also support for a transform called
"multiplyscale".  I do not know why this exsits.  It is functionally
no different from the transform called just "scale".  Weirdly the
description for "scale" says "Scale the input by a dimensionless factor"
but that's also untrue because it accepts as dimensionful quantity as
the factor...  I will make an upstream bug report.
This caught two more bugs, now fixed:

- the earthlocation tag was not handled properly by IsA
- when writing quantities scalar quantities with ndim=0 were written
  as an empty array
Found while adding a test to cover the dump/load code.
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.10345% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.55%. Comparing base (3a3c3e5) to head (923db75).

Files with missing lines Patch % Lines
src/yamlchan.c 64.77% 9 Missing and 22 partials ⚠️
src/object.c 77.77% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   61.13%   61.55%   +0.42%     
==========================================
  Files          83       83              
  Lines       96370    96376       +6     
  Branches    30543    30544       +1     
==========================================
+ Hits        58913    59324     +411     
+ Misses      21411    20910     -501     
- Partials    16046    16142      +96     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

In the majority if not all cases here the differences between versions
are mostly nominal (e.g. referencing a newer version of the 'quantity'
or base 'transform' schemas.  Some newer versions have also been updated
finally to be more flexible in the schemas they reference, using
wildcards, so as to require fewer schema version updates in the future.

Curiously, the following tags are recognized by AST but not actually
defined in any ASDF standard I can find:

- astropy/coordinates/frames/ecliptic-1.0.0
- astropy/coordinates/frames/altaz-1.0.0
- astropy/coordinates/frames/supergalactic-1.0.0

I'm not sure where these came problem.  Probably they *should* exist but
are not defined in

https://github.com/asdf-format/asdf-coordinates-schemas/tree/main/resources/schemas/frames

I left them for now just in case...
embray added a commit to asdf-format/libasdf-gwcs that referenced this pull request Jul 13, 2026
- Starlink/ast#37
- Starlink/ast#51
- Starlink/ast#66
- Starlink/ast#67

This includes the experimental SIMD support (enabled by default, and
support in AST for libfyaml, allowing us to drop the libyaml
requirement).

Also updates the minimum supported versions of the ASDF tags supported
by AST.
@embray embray mentioned this pull request Sep 1, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant